-
-
Notifications
You must be signed in to change notification settings - Fork 636
chore(deps): update rules_cc and protobuf versions #3367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update to the changelog.
Please update the PR description to explain why these bumps were necessary.
Mostly LGTM, sans the question and need to create deps.bzl function
py_repositories() | ||
|
||
# Needed for rules_cc 0.2.10 | ||
load("@bazel_features//:deps.bzl", "bazel_features_deps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of copy/pasting this same setup to every downstream workspace, create a second setup function and load and call that.
python/repositories_deps.bzl
which does the 4 lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried having a file load_cc_deps.bzl
that looks like this:
load("@bazel_features//:deps.bzl", "bazel_features_deps")
load("@rules_cc//cc:extensions.bzl", "compatibility_proxy_repo")
def add_cc_deps():
bazel_features_deps()
compatibility_proxy_repo()
but it did not work:
ERROR: Failed to load Starlark extension '@@bazel_features_version//:version.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
because bazel_features_deps()
call needs to happen before the load("@rules_cc//cc:extensions.bzl", "compatibility_proxy_repo")
and we cannot have load statements in starlark functions, or what you're suggesting is different?
- "--noenable_bzlmod" | ||
- "--build_tag_filters=-integration-test" | ||
# recent abseil-cpp requires C++17 which is not used by default in Bazel 7 | ||
- "--cxxopt=-std=c++17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit more about this? I'm guessing this is coming from protobuf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for protobuf 33.0 (and its dependency abseil-cpp) C++17 is the minimum supported version (https://github.com/google/oss-policies-info/blob/main/foundational-cxx-support-matrix.md)
I'm trying to add these flags to make it work with Bazel 7 but it is not straightforward as it is still failing for tests on windows (https://buildkite.com/bazel/rules-python-python/builds/13428#0199f84a-0a7c-4ed7-8b30-6ab8ef898f71) where -std=c++17
is not recognized. Do you have any suggestions for that?
Playing devil's advocate. This will be a breaking change for bazel 7 users who rely on protobuf. Should we cut a rules_python 2 where we remove protobuf dep instead? We can still merge the PR, but we could do a lightweight version 2 release with cleaning a few things up afterwards. |
These updates are mainly needed for compatibility with bazel 9:
rules_cc
to 0.2.10 to fix the cyclic dependency error while testing with bazel 9 pre-releaseprotobuf
to 33.0 which has load statements forrules_cc
needed for bazel 9